Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

[Dynamic Neighbor]: add patch to support dynamic neighbor configuration. #14

Merged
merged 6 commits into from
Jun 19, 2017

Conversation

sihuihan88
Copy link
Contributor

@sihuihan88 sihuihan88 commented Jun 9, 2017

This patch bases on FRRouting/frr@f14e6fd, FRRouting/frr@20eb886, FRRouting/frr@57e9ee0, FRRouting/frr@2aab8d2

To make it work in our code base, the major additional changes are:

  1. Add openbsd-queue.h in lib/
  2. Add bgp_af_index, afindex definition in bgpd/bgpd.h
  3. Add peer_af_create(), peer_af_find() in bgpd/bgpd.c
  4. Add MTYPE_BGP_PEER_AF in memtypes.c and memtypes.h
  5. Change bgp_debug_neighbor_events() to BGP_Debug() in bgpd.c and bgp_fsm.c
  6. Minor change of peer_delete() in bgpd.c
  7. Minor change of peer_and_group_lookup_vty(), peer_remote_as_vty () in bgp_vty.c
  8. Minor change of logging info.

@msftclas
Copy link

msftclas commented Jun 9, 2017

@sihuihan88,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

peer_delete (peer);
return -1;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same code 4 times. Probably it's better to introduce a static function here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is used to provide log info if debug_event is enabled. They are not exactly the same. So adding more detailed log info now for better troubleshooting.

@@ -220,6 +220,7 @@ bgp_accept (struct thread *thread)

peer1 = peer_lookup (NULL, &su);


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably it's better to remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Have updated accordingly.

@stcheng
Copy link

stcheng commented Jun 10, 2017

could you provide the patch info you reference? Add it to the commit message and also state what you have changed due to the patch conflicts.

@lguohan
Copy link
Contributor

lguohan commented Jun 13, 2017

can you check this commit? FRRouting/frr@20eb886

@lguohan
Copy link
Contributor

lguohan commented Jun 13, 2017

and this one. FRRouting/frr@57e9ee0

@lguohan
Copy link
Contributor

lguohan commented Jun 13, 2017

and this one FRRouting/frr@2aab8d2

{
if (BGP_DEBUG (events, EVENTS))
{
zlog_debug("BGP stop: %s (dynamic neighbor) deleted", peer->host);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-> zlog_info

check line 467: neighbor down event is logged as info level, this is also neighbor down event, should also logged as info level.

@lguohan
Copy link
Contributor

lguohan commented Jun 16, 2017

next time, you need to do a fork and https://help.github.com/articles/fork-a-repo/

peer_delete (peer);
return -1;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this logic can be merged into bgp_stop() as bgp_stop already has similar logic. We should enable code re-use.


peer_delete (peer);
return -1;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should move into bgp_stop()


peer_delete (peer);
return -1;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move into bgp_stop().

{
zlog_debug ("%s Dynamic Neighbor added, group %s count %d",
peer->host, group->name, dncount);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is neighbor add event, I think it should be info level, we should print it out when we enabled log neighbor change.

{
zlog_debug ("%s dropped from group %s, count %d",
peer->host, peer->group->name, dncount);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should do something simliar here.

  /* bgp log-neighbor-changes of neighbor Down */
  if (bgp_flag_check (peer->bgp, BGP_FLAG_LOG_NEIGHBOR_CHANGES))

zlog_info ("%%ADJCHANGE: neighbor %s Down %s", peer->host,
peer_down_str [(int) peer->last_reset]);

Copy link
Contributor

@lguohan lguohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly logging information related

@sihuihan88 sihuihan88 merged commit df6b709 into debian/0.99.24.1 Jun 19, 2017
@sihuihan88 sihuihan88 deleted the sihan/patch branch June 19, 2017 17:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants